-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Cleanup Webhook server setup #1504
✨ Cleanup Webhook server setup #1504
Conversation
Hi @aayushrangwala. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be some test that the directory has been properly cleaned up?
/ok-to-test |
Yes, definitely this needs to be part of unit tests and I was actually about to add too. But then I found there needs to be a comprehensive set of tests for checking the complete This seems to be the part of another PR to improve tests |
@aayushrangwala: The
Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/test pull-controller-runtime-test-master |
1 similar comment
/test pull-controller-runtime-test-master |
d260bf0
to
9cee39f
Compare
@aayushrangwala what about something like the following: Describe("Stop", func() {
Context("With existing cluster", func() {
It("should remove local serving certificate directory", func(done Done) {
env = &Environment{UseExistingCluster: pointer.Bool(true)}
_, err = env.Start()
Expect(err).NotTo(HaveOccurred())
Expect(env.Stop()).To(Succeed())
Expect(env.WebhookInstallOptions.LocalServingCertDir).ShouldNot(BeADirectory())
close(done)
}, 30)
})
}) I'm not sure if this is going to work, as it seems if useExisting := os.Getenv("USE_EXISTING_CLUSTER"); useExisting == "" {
Skip("USE_EXISTING_CLUSTER environment variable required")
} So at least one can run it locally until there is a CI facility to execute this test automatically. |
pkg/envtest/envtest_test.go
Outdated
@@ -65,15 +65,15 @@ var _ = Describe("Test", func() { | |||
Name: crd.GetName(), | |||
} | |||
var placeholder v1beta1.CustomResourceDefinition | |||
err := c.Get(context.TODO(), crdObjectKey, &placeholder) | |||
err = c.Get(context.TODO(), crdObjectKey, &placeholder) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err = c.Get(context.TODO(), crdObjectKey, &placeholder) | |
err := c.Get(context.TODO(), crdObjectKey, &placeholder) |
Please do not do scope changes for variables like this (This comment doesn't only apply to this line but to all places where you did that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Will change it back. Did that as the part of go-lint with no new variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what no new variable
is but IMHO it is best to keep the scope for variables as small as possible and this kind of change is the very opposite to that. Reduces the odds of accidentally propagating values to the wrong place and makes it easier for the garbage collector
885bb2b
to
6737748
Compare
Yeah, will add a case to check the cleanup but I think we dont need to worry about the existing cluster scenario, because as per the code even for the new cluster the same setup happens for Webhook install. So its better to keep it generic |
6737748
to
090a9cf
Compare
/test pull-controller-runtime-test-master |
3 similar comments
/test pull-controller-runtime-test-master |
/test pull-controller-runtime-test-master |
/test pull-controller-runtime-test-master |
- Called the webhook server cleanup function - Only ignore controlplane cleanup when using existing cluster - Added test to check the directory exists post stop
090a9cf
to
c34e747
Compare
@alvaroaleman Please re-review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@cndoit18: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aayushrangwala, alvaroaleman, cndoit18 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Called the webhook server cleanup function
Only ignore controlplane cleanup when using existing cluster
Added test to check the cleanup for Webhook after
Stop()
Fixes #1496